-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Include conversation parts in conversation #1349
base: develop
Are you sure you want to change the base?
Conversation
Just realized this would be terrible for perf on the index route. What we should do is perhaps make a diff view for the show route. Added |
238986f
to
ffc82fc
Compare
"read-at" => conversation.read_at, | ||
"status" => conversation.status, | ||
"inserted-at" => conversation.inserted_at, | ||
"updated-at" => conversation.updated_at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure to try to alpha order these so it's quicker to scan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Today is the day I start alpha ordering always 😆
} | ||
}, | ||
"included" => [%{ | ||
"attributes" => %{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably shift the object down and right one to match the style used earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
ffc82fc
to
d98b3a4
Compare
Although this is a small (and correct) change, I'd like some thoughts from @begedin on this so we can figure out how we generally want to approach these types of performance changes, including our "standards" for how preloads work, etc. |
The "include them on show, but do not on index" is probably the closest to ideal we can get with implicit includes. As I said already, ideally, the client would be able to specify when to include and when not to include, but outside of or until we have that, I'm not sure we can get better than this. Note that the spec does support explicitly specified includes as optional behavior: http://jsonapi.org/format/#fetching-includes Based on the spec we could write a catch-all "filter" to preload and add includes. It could parse the comma-separated My concerns are
If the current behavior (item 2 in concerns) works, then my vote is to merge this and devote some time on implementing explicit include behavior sooner rather than later, since we do not actually want to lose the certainty in acceptance tests. |
d98b3a4
to
22322b8
Compare
@begedin is this PR g2 merge? |
@snewcomer @joshsmith and I didn't get around to talking specifics yet - we may modify the approach a bit, but the work here is definitely useful and will be either merged fully as is, or in a modified form. Sorry to keep you waiting, but all of this is very much appreciated. |
4290c33
to
4f41648
Compare
e075407
to
6d6cc63
Compare
This results in significantly faster UI and smoother experience. Lmk what you think!